[HUDI-4465] Optimizing file-listing sequence of Metadata Table #6016

alexeykudinkin · 2022-07-01T00:23:07Z

Tips

Thank you very much for contributing to Apache Hudi.
Please review https://hudi.apache.org/contribute/how-to-contribute before opening a pull request.

What is the purpose of the pull request

This PR optimizes file-listing sequence of the Metadata Table to make sure it's on par or better than FS-based file-listing

Change log:

Cleaned up avoidable instantiations of Hadoop's Path
Replaced new Path w/ createUnsafePath where possible
Cached TimestampFormatter, DateFormatter for timezone
Avoid loading defaults in Hadoop conf when init-ing HFile reader
Avoid re-instantiating BaseTableMetadata twice w/in BaseHoodieTableFileIndex
Avoid looking up FileSystem for every partition when listing partitioned table, instead do it just once

Brief change log

See above

Verify this pull request

This pull request is already covered by existing tests, such as (please describe tests).

Committer checklist

Has a corresponding JIRA in PR title & commit
Commit message is descriptive of the change
CI is green
Necessary doc changes done or have another open PR
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.

nsivabalan · 2022-07-19T09:24:03Z

hey @alexeykudinkin : can you link the right jira for the patch.

codope · 2022-07-28T00:15:14Z

...rc/test/java/org/apache/hudi/table/functional/TestHoodieSparkMergeOnReadTableCompaction.java

+        .withIndexConfig(HoodieIndexConfig.newBuilder().withIndexType(HoodieIndex.IndexType.BUCKET).withBucketNum("1").build())
+        .build();
+
+    Properties props = getPropertiesForKeyGen(true);


pass HoodieTableConfig.POPULATE_META_FIELDS.defaultValue() instead of hard-coding true?

codope · 2022-07-28T00:22:29Z

hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableMetaClient.java

    // Once meta fields are disabled, it cant be re-enabled for a given table.
    if (!getTableConfig().populateMetaFields()
-        && Boolean.parseBoolean((String) properties.getOrDefault(HoodieTableConfig.POPULATE_META_FIELDS.key(), HoodieTableConfig.POPULATE_META_FIELDS.defaultValue()))) {
+        && Boolean.parseBoolean((String) properties.getOrDefault(HoodieTableConfig.POPULATE_META_FIELDS.key(), HoodieTableConfig.POPULATE_META_FIELDS.defaultValue().toString()))) {


is it necessary? it's already being type cast to String

hudi-common/src/main/java/org/apache/hudi/BaseHoodieTableFileIndex.java

codope · 2022-07-28T00:58:52Z

hudi-common/src/main/java/org/apache/hudi/hadoop/CachingPath.java

+  }
+
+  /**
+   * TODO elaborate


todo? javadoc only right?

codope · 2022-07-28T01:05:36Z

hudi-common/src/main/java/org/apache/hudi/common/table/log/block/HoodieHFileDataBlock.java

-    FileSystem fs = FSUtils.getFs(pathForReader.toString(), new Configuration());
    // Read the content
-    HoodieHFileReader<IndexedRecord> reader = new HoodieHFileReader<>(fs, pathForReader, content, Option.of(writerSchema));
+    HoodieHFileReader<IndexedRecord> reader = new HoodieHFileReader<>(null, pathForReader, content, Option.of(writerSchema));


This could affect HFile reading. I believe there is some validation in HFile system or HFile's reader context for fs to be non-null. I think we should still pass fs and still keep this line in HoodieHFileUtils#createHFileReader:

Configuration conf = new Configuration(false);

Yeah, i checked it and it actually doesn't use fs at all

codope · 2022-07-28T01:19:50Z

hudi-common/src/main/java/org/apache/hudi/BaseHoodieTableFileIndex.java

+  private List<String> getAllPartitionPathsUnchecked() {
+    try {
+      if (partitionColumns.length == 0) {
+        return Collections.singletonList("");


Should it be Collections.emptyList()?

Non-partitioned table has exactly one partition, which we designate w/ ""

codope · 2022-07-28T01:21:49Z

hudi-common/src/main/java/org/apache/hudi/metadata/BaseTableMetadata.java

  public Map<String, FileStatus[]> getAllFilesInPartitions(List<String> partitions)
      throws IOException {
+    if (partitions.isEmpty()) {
+      return Collections.emptyMap();


If BaseHoodieTableFileIndex#getAllPartitionPathsUnchecked returns Collections.singletonList("") then should we add an entry for "" in the map, or rather make getAllPartitionPathsUnchecked return Collections.emptyList()

Agree, this is somewhat dissonant, but that's just the way things are -- for non-partitioned tables it's assumed that the only partition that is there has to be identified by ""

codope · 2022-07-28T01:43:39Z

hudi-common/src/test/java/org/apache/hudi/common/testutils/HoodieTestUtils.java

+
+    String keyGen = properties.getProperty("hoodie.datasource.write.keygenerator.class");
+    if (!Objects.equals(keyGen, "org.apache.hudi.keygen.NonpartitionedKeyGenerator")) {
+      builder.setPartitionFields("some_nonexistent_field");


extract to constant to standardize across tests?

I don't think we should actually standardize on this one, it's just to stop the bleeding in misconfigured tests

codope · 2022-07-28T01:44:32Z

...-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/SparkHoodieTableFileIndex.scala

+
+  private def shouldValidatePartitionColumns(spark: SparkSession): Boolean = {
+    // NOTE: We can't use helper, method nor the config-entry to stay compatible w/ Spark 2.4
+    spark.sessionState.conf.getConfString("spark.sql.sources.validatePartitionColumns", "true").toBoolean


Should this go into Spark2Adapter or Spark2ParsePartitionUtil?

We don't need to

codope

LGTM. Can you please rebase? We can land once the CI is green.

…n the hot-path

Tidying up

… reflection in the hot-path

…bleFileIndex`

…cing it w/ - `CachingPath` object - Invoking more performnat unsafe ctors/utils

Use `CachingPath` in `SparkHoodieTableFileIndex`; Tidying up;

Refactored `SerializablePath` to serialize `URI` instead (to avoid parsing, when deser); Tidying up

partition-path fields are non-empty

codope · 2022-09-05T07:22:21Z

hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/keygen/SimpleKeyGenerator.java

+    // Make sure key-generator is configured properly
+    ValidationUtils.checkArgument(recordKeyField == null || !recordKeyField.isEmpty(),
+        "Record key field has to be non-empty!");
+    ValidationUtils.checkArgument(partitionPathField == null || !partitionPathField.isEmpty(),


Should the validation message be more user-friendly? Let's say
"Partition path field has to be non-empty! For non-partitioned table, set key generator class to NonPartitionedKeyGenerator".
Also, why are these validations only added for SimpleKeyGenerator? Why not other keygens as well?

I don't think it makes sense to put suggestions into exception messages -- exception messages should be focused on the problem triggering it, rather than on potential to remedy it (empty partition-path field is usually a sign of misconfiguration, since there's no default value, meaning that user passes "" explicitly)

Fair enough.
Should we add these validations to other keygens as well?

Discussed offline. It will be taken up separately. @alexeykudinkin in case if you have a JIRA, please link it here. For simple keygen we need the validation because of misconfiguration of some tests that were passing “” as partition fields.

...rk-datasource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestTimeTravelQuery.scala

hudi-bot · 2022-09-07T04:36:52Z

CI report:

46e53b5 Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

@hudi-bot run azure re-run the last Azure build

xushiyan · 2022-10-26T09:44:57Z

hudi-common/src/main/java/org/apache/hudi/BaseHoodieTableFileIndex.java

-    List<String> matchedPartitionPaths = FSUtils.getAllPartitionPaths(engineContext, metadataConfig, basePath)
+    List<String> matchedPartitionPaths = getAllPartitionPathsUnchecked()


this change affects partitioned tables that meet both of these conditions 1) hoodie.table.partition.fields not present in table config, and 2) metadata disabled. getAllPartitionPathsUnchecked() treats them as non-partitioned table, and resulted in not loading any records.

The core of the problem is that hoodie.table.partition.fields has to be properly configured -- the table would be considered non-partitioned by some parts of the code (outside of this one) so we need to make sure this is set properly.

alexeykudinkin force-pushed the ak/mtd-fl-lst-opt branch from bf5f6ae to ecf4114 Compare July 1, 2022 00:25

codope added priority:blocker Production down; release blocker metadata area:performance Performance optimizations labels Jul 12, 2022

codope assigned yihua Jul 20, 2022

alexeykudinkin changed the title ~~[WIP] Optimizing file-listing sequence of Metadata Table~~ [HUDI-4465] Optimizing file-listing sequence of Metadata Table Jul 25, 2022

alexeykudinkin force-pushed the ak/mtd-fl-lst-opt branch from ecf4114 to 0ae8d0e Compare July 25, 2022 17:34

codope added priority:critical Production degraded; pipelines stalled and removed priority:blocker Production down; release blocker labels Jul 25, 2022

xushiyan assigned alexeykudinkin Jul 25, 2022

alexeykudinkin force-pushed the ak/mtd-fl-lst-opt branch from 0ae8d0e to 6194091 Compare July 26, 2022 20:36

xushiyan added priority:blocker Production down; release blocker and removed priority:critical Production degraded; pipelines stalled labels Jul 27, 2022

alexeykudinkin force-pushed the ak/mtd-fl-lst-opt branch from 40e7639 to 178c9f7 Compare July 27, 2022 23:30

codope reviewed Jul 28, 2022

View reviewed changes

alexeykudinkin force-pushed the ak/mtd-fl-lst-opt branch 3 times, most recently from b038d97 to 705660e Compare July 28, 2022 23:11

codope added priority:critical Production degraded; pipelines stalled and removed priority:blocker Production down; release blocker labels Aug 5, 2022

codope approved these changes Sep 2, 2022

View reviewed changes

Alexey Kudinkin added 8 commits September 2, 2022 12:05

Replaced string paht w/ Hadoop's Path to avoid costly conversions i…

885619c

…n the hot-path

Avoid fetching FileSystem.get since it's unused;

f2ad090

Tidying up

Cache DateFormatter/TimestampFormatter to avoid loading these t/h…

a6c3130

… reflection in the hot-path

Avoid re-instantiating HoodieTableMetadata twice w/in `BaseHoodieTa…

c812871

…bleFileIndex`

Fixing compilation

6c96323

Avoid creating costly Hadoop's Path object wherever possible, repla…

a3945f2

…cing it w/ - `CachingPath` object - Invoking more performnat unsafe ctors/utils

Fixing compilation

8bc2456

Avoid loading defaults in Hadoop conf when init-ing HFile reader;

8e10cb8

Use `CachingPath` in `SparkHoodieTableFileIndex`; Tidying up;

Alexey Kudinkin added 10 commits September 2, 2022 12:05

Reverting experimental changes

350c16e

Fixing compilation (for Spark 2.4)

6508ce9

Make CachingPath non Serializable;

2ffe683

Refactored `SerializablePath` to serialize `URI` instead (to avoid parsing, when deser); Tidying up

Fixed tests

de08ca3

Fixed invalid cast in HoodieTableMetaClient

f88229b

Fixing tests w/ invalid configuration

068a110

Fixing more tests w/ invalid configuration

3db948f

Fixed configs in IT to properly mirror configs for DS cases

56d5176

Tidying up

409066f

Carve out exception for MT to avoid treating it as non-partitioned one

95ce817

alexeykudinkin force-pushed the ak/mtd-fl-lst-opt branch from 705660e to 95ce817 Compare September 2, 2022 19:05

Alexey Kudinkin added 3 commits September 2, 2022 20:03

Fixed SimpleKeyGenerator to assert that configured primary-key and

d448c74

partition-path fields are non-empty

Fixed improperly configured test

ea2c947

Fixed invalid partition-path configs in TestHoodieDeltaStreamer suite

8be611a

codope reviewed Sep 5, 2022

View reviewed changes

Rebased TimeTravelQuery test to use Simple KG instead

46e53b5

alexeykudinkin force-pushed the ak/mtd-fl-lst-opt branch from be9c519 to 46e53b5 Compare September 7, 2022 00:48

codope approved these changes Sep 9, 2022

View reviewed changes

codope merged commit 4af60dc into apache:master Sep 9, 2022

xushiyan reviewed Oct 26, 2022

View reviewed changes

xushiyan mentioned this pull request Oct 26, 2022

[HUDI-5097] Fix partition reading without partition fields table config #7069

Merged

4 tasks

jonvex mentioned this pull request May 15, 2023

[HUDI-915] Add missing partititonpath to records COW #8666

Merged

4 tasks

hudi-bot mentioned this pull request Nov 30, 2025

Get rid of "some_nonexistent_field" used in some tests as a fake partition column #16710

Closed

		List<String> matchedPartitionPaths = FSUtils.getAllPartitionPaths(engineContext, metadataConfig, basePath)
		List<String> matchedPartitionPaths = getAllPartitionPathsUnchecked()

[HUDI-4465] Optimizing file-listing sequence of Metadata Table #6016

[HUDI-4465] Optimizing file-listing sequence of Metadata Table #6016

Uh oh!

Conversation

alexeykudinkin commented Jul 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tips

What is the purpose of the pull request

Brief change log

Verify this pull request

Committer checklist

Uh oh!

nsivabalan commented Jul 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codope left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hudi-bot commented Sep 7, 2022

CI report:

Uh oh!

xushiyan Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

alexeykudinkin commented Jul 1, 2022 •

edited

Loading

xushiyan Oct 26, 2022 •

edited

Loading